feat: SwiftUI View Rendering Instrumentation #20

MustafaHaddara · 2024-10-30T20:20:55Z

Which problem is this PR solving?

Closes #

Short description of the changes

SwiftUI rendering is very interesting!

This PR currently has a whole bunch of instrumentation and logging which makes it easier to see what's going on

This PR intentionally does not ship a view modifier form of the instrumentation. This is because ViewModifiers receive a Content struct (vs. Views that receive an @ViewBuilder () -> Content callback!); that is: ViewModifiers receive a view after it has been initialized, and therefore have no way to retroactively start a span before the view begins initializing.

Compare the spans/log output from using the wrapper view form:

with the view modifier form:

And, now that we've settled on the wrapper form, here's the kinds of traces we're producing:

How to verify that this has the expected result

beekhc

Some early feedback.

Examples/SmokeTest/SmokeTest/ContentView.swift

Sources/Honeycomb/HoneycombInstrumentedView.swift

…-swift into mh/instrumented-view

beekhc

Overall, this looks great, and I just have a bunch of nits. But also, are you going to add some SmokeTest UI tests and assertions?

beekhc · 2024-11-14T22:40:25Z

Examples/SmokeTest/SmokeTest/ContentView.swift

+
+            ViewInstrumentationView()
+                .padding()
+                .tabItem { Label("View Instrumentation", systemImage: "ruler") }


It's really hard to resist the urge to bikeshed what icon to use for each tab 😆

beekhc · 2024-11-14T22:43:17Z

Examples/SmokeTest/SmokeTest/ViewInstrumentationView.swift

+                } onEditingChanged: { editing in
+                    isEditing = editing
+                    if !editing {
+                        delay = sliderDelay


I really thought that since the value of the slider was a $ binding that you would just be able to have Slider(value: $delay, and not have any onEditingChanged: handler. If this is necessary, could you add a comment explaining why we need all three state variables?

yeah the idea was that we'd be able to adjust the slider but the rest of the view wouldn't re-render until you stopped editing it.

BUT clearly I wasn't paying enough attention and that wasn't actually working. I'll restructure it and add a comment.

works now:

Screen.Recording.2024-11-15.at.2.48.07.PM.mp4

(yes i know the simulator has a screen recorder but i wanted to capture my mouse cursor lol)

beekhc · 2024-11-14T22:48:43Z

Examples/SmokeTest/SmokeTest/ViewInstrumentationView.swift

+
+private func timeConsumingCalculation(_ delay: Double) -> String {
+    print("starting time consuming calculation")
+    sleep(UInt32(delay))


sleep() is the POSIX function that takes a whole number of seconds, but it would be better to use the Swift function that takes a TimeInterval: Thread.sleep(forTimeInterval: delay)

beekhc · 2024-11-14T22:50:18Z

Examples/SmokeTest/SmokeTest/ViewInstrumentationView.swift

+    }
+}
+
+private func timeConsumingCalculation(_ delay: Double) -> String {


Rather than Double, it would be more idiomatic and self-documenting to use TimeInterval, which is an alias for Double. I would try to use it as much as possible. Like, if we still need two state variables above, probably at least the delay one should be TimeInterval too.

beekhc · 2024-11-14T22:50:56Z

Sources/Honeycomb/HoneycombInstrumentedView.swift

+    }
+
+    var body: some View {
+        print("\(name) body started rendering")


Don't forget to remove print statements.

Do we have any kind of debug logging infrastructure? I've found these logs to be helpful when I play with the smoke tests.

beekhc · 2024-11-14T22:52:17Z

Sources/Honeycomb/HoneycombInstrumentedView.swift

+
+            print("\(name) content appeared")
+            span.setAttribute(key: "TotalDuration", value: Date().timeIntervalSince(initTime))
+            span.end(time: Date())


Would it be better to capture the Date() once and use the same value on both these lines?

beekhc · 2024-11-14T22:54:11Z

Sources/Honeycomb/HoneycombInstrumentedView.swift

+            print("\(name) content appeared")
+            span.setAttribute(key: "TotalDuration", value: Date().timeIntervalSince(initTime))
+            span.end(time: Date())
+            bodySpan.end(time: endTime)


I'm a little confused about this. Is there a reason we can't do this on line 40, right after endTime is set?

I wanted it to remain active in context, I put some comments in the code that should it explain it better.

thanks for the comment. very helpful.

MustafaHaddara · 2024-11-18T16:18:13Z

smoke-tests/test_helpers/utilities.bash

@@ -20,7 +20,7 @@ span_attributes_for() {
 #   $3 - attribute key
 #   $4 - attribute type
 attribute_for_span_key() {
-	attributes_from_span_named $1 $2 | \
+	attributes_from_span_named "$1" "$2" | \


We're emitting spans named View Render so these args need to be quoted for our utils to work correctly.

beekhc

This is great. I made a few comments for future discussion, but they shouldn't prevent us from submitting this.

beekhc · 2024-11-18T17:53:17Z

README.md

+`View Render` spans encompass the entire rendering process, from initialization to appearing on screen. They include the following attributes:
+- `ViewName` (string): the name passed to `HoneycombInstrumentedView`
+- `RenderDuration` (double): amount of time to spent initializing the contents of `HoneycombInstrumentedView`
+- `TotalDuration` (double): amount of time from when `HoneycombInstrumentedView::body()` is called to when the contents appear on screen


Nit: it's ., not ::. This isn't C++. :-)

beekhc · 2024-11-18T18:03:59Z

Examples/SmokeTest/SmokeTest/ViewInstrumentationView.swift

+struct DelayedSlider: View {
+    @State private var sliderDelay = 2.0
+    @State private var isEditing = false
+    var delay: Binding<Double>


I love it! <3

beekhc · 2024-11-18T18:09:28Z

README.md

+
+Specifically, it will emit 2 kinds of span for each view that is wrapped:
+
+`View Render` spans encompass the entire rendering process, from initialization to appearing on screen. They include the following attributes:


I'm surprised there aren't specific semantic conventions around spaces and capitalization in span names, but it appears there are not: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.37.0/specification/trace/api.md#span

Anyway, this is fine for now, and we can always tweak them later, since we're still in experimental alpha mode. We should probably just be consistent with our web sdk.

beekhc · 2024-11-18T18:09:59Z

Sources/Honeycomb/HoneycombInstrumentedView.swift

+            print("\(name) content appeared")
+            span.setAttribute(key: "TotalDuration", value: Date().timeIntervalSince(initTime))
+            span.end(time: Date())
+            bodySpan.end(time: endTime)


thanks for the comment. very helpful.

beekhc · 2024-11-18T18:11:47Z

smoke-tests/smoke-e2e.bats

+@test "Render Instrumentation attributes are correct" {
+  # we got the spans we expect
+  result=$(span_names_for "@honeycombio/instrumentation-view" | sort | uniq -c)
+  assert_equal "$result" '   7 "View Body"


We should keep an eye on this and make sure it isn't flaky. I'm a little worried about something getting rendered an extra time for whatever reason, and the counts being off.

MustafaHaddara added 2 commits November 1, 2024 12:55

view rendering instrumentation via ViewModifier

3ef09db

Switch to wrapper View for instrumentation

7f08ddf

MustafaHaddara force-pushed the mh/instrumented-view branch from 844bdc7 to 7f08ddf Compare November 1, 2024 16:55

beekhc reviewed Nov 5, 2024

View reviewed changes

MustafaHaddara added 8 commits November 13, 2024 11:35

update active spans

5d573f9

use correct tracer

ac81129

use double-precision seconds as units for duration attributes

3ebd02a

Merge branch 'main' of github.com:honeycombio/honeycomb-opentelemetry…

6a6b438

…-swift into mh/instrumented-view

move to separate tab

7b422ed

add toggles to control slow rendering

a338dc4

lint

4b8e406

set default to 2s sleep now that it's turned off automatically

c8a031a

MustafaHaddara marked this pull request as ready for review November 13, 2024 21:30

MustafaHaddara requested a review from a team as a code owner November 13, 2024 21:30

MustafaHaddara changed the title ~~feat: [WIP] view rendering instrumentation~~ feat: view rendering instrumentation Nov 13, 2024

MustafaHaddara changed the title ~~feat: view rendering instrumentation~~ feat: SwiftUI View Rendering Instrumentation Nov 13, 2024

MustafaHaddara added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Nov 13, 2024

remove unnecessary import

329c61f

beekhc reviewed Nov 14, 2024

View reviewed changes

MustafaHaddara added 2 commits November 15, 2024 14:47

PR comments: remove print statements, add comments, restructure slider

b394dfc

Add smoke tests for view instrumentation

2aa90bf

MustafaHaddara commented Nov 18, 2024

View reviewed changes

MustafaHaddara added 2 commits November 18, 2024 11:18

lint

b4e5164

document spans emitted by view instrumentation

affe0cf

beekhc mentioned this pull request Nov 18, 2024

feat: UIKit Navigation instrimentation #22

Merged

beekhc approved these changes Nov 18, 2024

View reviewed changes

this isn't C++ :)

f5cf382

MustafaHaddara merged commit 71410b9 into main Nov 18, 2024
6 checks passed

MustafaHaddara deleted the mh/instrumented-view branch November 18, 2024 20:24

MustafaHaddara mentioned this pull request Dec 11, 2024

feat: add render instrumentation honeycombio/honeycomb-opentelemetry-android#20

Draft

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SwiftUI View Rendering Instrumentation #20

feat: SwiftUI View Rendering Instrumentation #20

MustafaHaddara commented Oct 30, 2024 •

edited

Loading

beekhc left a comment

beekhc left a comment

beekhc Nov 14, 2024

beekhc Nov 14, 2024

MustafaHaddara Nov 15, 2024

MustafaHaddara Nov 15, 2024

beekhc Nov 14, 2024

beekhc Nov 14, 2024

beekhc Nov 14, 2024

MustafaHaddara Nov 15, 2024

beekhc Nov 14, 2024

beekhc Nov 14, 2024

MustafaHaddara Nov 15, 2024

beekhc Nov 18, 2024

MustafaHaddara Nov 18, 2024

beekhc left a comment

beekhc Nov 18, 2024

beekhc Nov 18, 2024

beekhc Nov 18, 2024

beekhc Nov 18, 2024

beekhc Nov 18, 2024


		Specifically, it will emit 2 kinds of span for each view that is wrapped:

		`View Render` spans encompass the entire rendering process, from initialization to appearing on screen. They include the following attributes:

feat: SwiftUI View Rendering Instrumentation #20

feat: SwiftUI View Rendering Instrumentation #20

Conversation

MustafaHaddara commented Oct 30, 2024 • edited Loading

Which problem is this PR solving?

Short description of the changes

How to verify that this has the expected result

beekhc left a comment

Choose a reason for hiding this comment

beekhc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beekhc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MustafaHaddara commented Oct 30, 2024 •

edited

Loading